Skip to content

Show the warnings the compiler already had - #467

Merged
jeremymanning merged 2 commits into
mainfrom
feat/surface-validation-warnings
Aug 4, 2026
Merged

Show the warnings the compiler already had#467
jeremymanning merged 2 commits into
mainfrom
feat/surface-validation-warnings

Conversation

@jeremymanning

Copy link
Copy Markdown
Member

The second half of #465.

The defect

A pipeline could print

✓ p.yaml is valid

while carrying a warning saying a reference could not be checked — and then fail at run time on exactly that reference. The compiler had collected the finding all along; nothing displayed it. It went to the log stream, which validate does not write to and a script capturing stdout never sees.

As the review put it: logging is not an adequate user-facing warning channel.

The human channel

✓ p.yaml is valid with 2 warnings
  pipeline: uo
  tasks: 2
    - make
    - use  <- make

warning[data_flow_undefined_output] content:
  Task 'make' does not declare its outputs, so 'path' cannot be checked until it runs

Informational findings stay out. "Tool is available" and "execution order computed" are not things anyone needs told, and printing them would turn the channel into noise — at which point it stops being read, which is how this started.

Warnings do not fail validation. Making them errors would reject pipelines that run correctly, which is the false-positive class removed in #448/#450/#461.

The structured channel

--json emits the whole result as one document — and only that document, since anything appended makes it unparseable, a mistake caught in the catalogue report earlier this week.

Field names are the interface, so nothing has to parse English to learn what is wrong:

{
  "code": "data_flow_undefined_output",
  "severity": "warning",
  "category": "data_flow",
  "step": "use",
  "parameter_path": "content",
  "referenced_step": "make",
  "referenced_field": "path",
  "message": "...",
  "suggestions": []
}

referenced_step and referenced_field are new: data-flow findings carried them only inside the prose message. JSON is emitted for invalid pipelines too, so a consumer does not switch parsers based on the outcome.

A latent hazard found on the way

--json initially printed the configured provider list and exited, emitting no JSON at all. It looked like a click registration problem; --json was registered correctly.

cli.py defined def list() at module scope, rebinding the builtin for the whole file. A click Command is callable, so list(issue.suggestions or []) did not build a list — it invoked the keys list command, printed the providers, and exited before its own output.

Fixed at the root: the function is list_keys with the CLI name passed to the decorator, so the command name is unchanged. A test now refuses any module-level name in cli.py that shadows a builtin, because anything named after a builtin here is one call away from repeating this.

Verification

  • 11 tests across both channels, plus the shadowing guard.
  • Mutations, each caught: findings collected but not printed (3 failures); the shadowing reintroduced (guard fires).
  • Blocking suite 842 passed, 0 failed. Lint clean.
  • keys list still works under its CLI name, with a test.

🤖 Generated with Claude Code

jeremymanning and others added 2 commits August 3, 2026 23:43
A pipeline could print

    ✓ p.yaml is valid

while carrying a warning that a reference could not be checked, and then fail
at run time on exactly that reference (#465). The compiler had collected the
finding all along; nothing displayed it. It went to the log stream, which
validate does not write to and a script capturing stdout never sees.

A successful validate now prints its warnings, with their codes, and says how
many in the summary line. Informational findings stay out: 'tool is available'
and 'execution order computed' are not things anyone needs told, and reporting
that would turn the channel into noise.

--json emits the whole result as one document, findings included, with field
names that are an interface rather than English to be parsed: code, severity,
category, step, parameter_path, referenced_step, referenced_field, message,
suggestions. Data-flow findings now carry the referenced step and field
structurally instead of only in prose.

Along the way: cli.py defined 'def list()' at module scope, rebinding the
builtin for the whole file. A click Command is callable, so 'list(...)' did
not build a list -- it invoked the command, printed the configured providers
and exited before its own output. --json emitted nothing and looked like a
registration problem. The function is now list_keys with the CLI name passed
to the decorator, and a test refuses any module-level name in cli.py that
shadows a builtin.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_keys_list_is_still_reachable ran 'orchestrator keys list', which reports
the user's configured providers and exits non-zero when there are none. It
passed on my machine because I have keys and failed in CI because CI has
none -- by design; the default workflow is hermetic and carries no secrets.

The test was asserting the wrong thing anyway. What matters is that renaming
the function to list_keys did not rename the CLI command, and that is a
question about registration: it is now asked of the click group directly, and
an e2e companion uses --help, which resolves the command without needing any
credential to exist.

Verified by running the suite with the provider variables unset and HOME
pointed at an empty directory, so ~/.orchestrator/.env is absent too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jeremymanning
jeremymanning merged commit 7e7d716 into main Aug 4, 2026
11 checks passed
@jeremymanning
jeremymanning deleted the feat/surface-validation-warnings branch August 4, 2026 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant